Fix unreliable ASTJson tests #12696

wechman · 2022-02-17T17:51:49Z

ASTJson test variants are generated based on JSON file availability. Then they are validated against "failAfter" marker in a solidity source file. If the "failAfter" marker is not here all generated test variants are accepted. Otherwise, compiler state in test variant is compared against "failAfter" marker. If it is later in compilation steps order, the test is interrupted with exception.

test/libsolidity/ASTJSONTest.cpp

cameel

I have some remarks but overall I like the direction it's going in.

test/libsolidity/ASTJSONTest.cpp

wechman · 2022-03-04T14:01:08Z

I squashed commits and rebased them on the top of the develop branch.
@Marenz @cameel @chriseth do you see anything that needs to be addressed before we merge?

ekpyron · 2022-03-07T09:23:52Z

test/libsolidity/ASTJSONTest.cpp

+
+	if (m_variants.empty())
+		BOOST_THROW_EXCEPTION(runtime_error("Missing file with expected result for: \"" + _filename + "\"."));
+
 	ifstream file(_filename);


Is there any reason why all the stuff starting from here cannot be done by TestCaseReader instead? Not that it's strongly related to the issue at hand, but still...

ekpyron · 2022-03-07T09:58:01Z

What will happen, if I add failAfter: Parsed to a test, but it still has the old expectation file?
Or conversely, what will happen if failAfter: Parsed is present, but no _parseOnly file is present?
It might in general help if the PR description explained the intended new logic.

To me it would seem that, ideally, the solidity source file fully determines what variants are expected with its settings and a run of isoltest will verify that the appropriate expectation files are there and only those - and error out specifically if the set of expectation files doesn't match... but from a quick glance, it looks like the PR runs variants based on which files are present?

But I may have missed some discussion on this.

wechman · 2022-03-07T12:23:34Z

@ekpyron Yes, the idea was to run variants based on which files are present. I am not sure if we can depend on *.sol file only. In fail_after_parsing test we expect a compilation fail after parsing. But even so, in the second test variant, a result AST is enriched by "exportedSymbols". Do you think the second variant makes not sense?

ekpyron · 2022-03-07T12:32:03Z

@ekpyron Yes, the idea was to run variants based on which files are present. I am not sure if we can depend on *.sol file only. In fail_after_parsing test we expect a compilation fail after parsing. But even so, in the second test variant, a result AST is enriched by "exportedSymbols". Do you think the second variant makes not sense?

I don't think there is any way to make the compiler output an AST on the fail_after_parsing test without specifying --stop-after parsing (which will lead to the _parseOnly AST). So the second variant does not really make sense - it just tests some partial intermediate result that will never actually occur anywhere.

wechman · 2022-03-07T12:50:07Z

Thank you for the quick response! It is all clear now. I will update code to validate the test variants based on the "failAfter" marker.

wechman · 2022-03-07T13:58:35Z

@ekpyron I updated PR with JSONs/variants validation. Please let me know if this is what you expected to see.

test/libsolidity/ASTJSON/fail_after_parsing.sol

test/libsolidity/ASTJSONTest.cpp

ekpyron

I'm still wondering about #12696 (comment) - but we don't need to change this in the same PR, even if we want to change it, I guess.

Also I'd still consider always expecting and checking a parseOnly file, but arguably it's also fine to merely check it if it's there...

So since regardless of either of that, this PR is definitely an improvement, I'm approving.

chriseth · 2022-03-14T13:14:37Z

Please squash

wechman · 2022-03-14T13:56:47Z

Please squash

done

wechman force-pushed the unreliableAstJsonTests branch from babfb44 to 2ac041e Compare February 18, 2022 08:15

wechman dismissed a stale review via ff44c8a February 21, 2022 11:26

wechman force-pushed the unreliableAstJsonTests branch from 2ac041e to ff44c8a Compare February 21, 2022 11:26

Marenz suggested changes Feb 21, 2022

View reviewed changes